feat(shared): add ctx.metadata.delete(), clear(), get(), and current fixes NV-7501#10971
feat(shared): add ctx.metadata.delete(), clear(), get(), and current fixes NV-7501#10971
Conversation
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
📝 WalkthroughWalkthroughAdds action-discriminated metadata signals ( ChangesMetadata Signal Actions (single coherent DAG)
Sequence DiagramsequenceDiagram
participant Agent
participant AgentContext as Agent Context
participant SignalQueue as Signal Queue
participant Validator as Signal Validator
participant Service as Metadata Service
Agent->>AgentContext: metadata.set(key, value)
AgentContext->>SignalQueue: enqueue {type:'metadata', action:'set', key, value}
Agent->>AgentContext: metadata.delete(key)
AgentContext->>SignalQueue: enqueue {type:'metadata', action:'delete', key}
Agent->>AgentContext: metadata.clear()
AgentContext->>SignalQueue: enqueue {type:'metadata', action:'clear'}
Agent->>AgentContext: reply() / flush()
AgentContext->>Validator: validate metadata signals
Validator->>Validator: action='set' -> require key & value
Validator->>Validator: action='delete' -> require key
Validator->>Validator: action='clear' -> always valid
Validator-->>Service: normalized ops (MetadataOp[])
Service->>Service: apply clear / delete / set in op order
Service-->>Agent: persisted metadata update / activity
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Microsoft Presidio Analyzer (2.2.362)apps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.tsMicrosoft Presidio Analyzer failed to scan this file Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/framework/src/resources/agent/agent.types.ts (1)
171-174: Treat the newAgentContext.metadatamethods as a semver-significant API change.Adding required members to a public exported interface can break downstream consumer mocks/implementations at compile time. Please make sure this ships under a release that allows breaking type-surface changes, or keep the addition backward-compatible.
As per coding guidelines, "packages/**/*: Treat all exported symbols in packages as public API; follow semver conventions with breaking changes requiring major bumps, new exports as minor versions, and fixes as patches."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/framework/src/resources/agent/agent.types.ts` around lines 171 - 174, You added required methods to the public AgentContext.metadata interface (set, delete, clear), which is a semver-breaking type change; either revert them or make the change backward-compatible by marking those members optional (metadata.set?, metadata.delete?, metadata.clear?) and ensure any concrete implementations (the classes/factories that construct AgentContext) provide no-op or real implementations so consumers compiling against older mocks won't break; alternatively, if you intend the breaking change, document it and ship under a major-version bump per semver.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts`:
- Around line 55-60: The metadata_delete and metadata_clear branches currently
accept extra unrelated fields; update their validation to reject extra
properties by ensuring the signal object only contains the allowed keys: for
metadata_delete require signal.type === 'metadata_delete' AND
isValidMetadataSignalKey(signal.key) AND Object.keys(signal) is exactly
['type','key'], and for metadata_clear require signal.type === 'metadata_clear'
AND Object.keys(signal) is exactly ['type'] (do the same corresponding stricter
check in the other branch around lines 74-75). Use the existing
isValidMetadataSignalKey helper and the signal.type discriminator when
implementing the exact-key checks so the accepted shape matches
defaultMessage().
---
Nitpick comments:
In `@packages/framework/src/resources/agent/agent.types.ts`:
- Around line 171-174: You added required methods to the public
AgentContext.metadata interface (set, delete, clear), which is a semver-breaking
type change; either revert them or make the change backward-compatible by
marking those members optional (metadata.set?, metadata.delete?,
metadata.clear?) and ensure any concrete implementations (the classes/factories
that construct AgentContext) provide no-op or real implementations so consumers
compiling against older mocks won't break; alternatively, if you intend the
breaking change, document it and ship under a major-version bump per semver.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 286ff233-6f49-4c51-bbfd-b19df3319ff4
📒 Files selected for processing (8)
apps/api/src/app/agents/dtos/agent-reply-payload.dto.tsapps/api/src/app/agents/services/agent-conversation.service.tsapps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.tspackages/framework/src/index.tspackages/framework/src/resources/agent/agent.context.tspackages/framework/src/resources/agent/agent.test.tspackages/framework/src/resources/agent/agent.types.tspackages/framework/src/resources/agent/index.ts
| if (signal.type === 'metadata_delete') { | ||
| return isValidMetadataSignalKey(signal.key); | ||
| } | ||
|
|
||
| if (signal.type === 'metadata_clear') { | ||
| return true; |
There was a problem hiding this comment.
Reject extra fields on metadata_delete and metadata_clear.
These branches currently accept payloads with unrelated fields like value, workflowId, or payload, so malformed signals still validate and the server silently ignores the extra data. Tighten the discriminator checks so the accepted shape matches the contract in defaultMessage().
Suggested fix
if (signal.type === 'metadata_delete') {
- return isValidMetadataSignalKey(signal.key);
+ return (
+ isValidMetadataSignalKey(signal.key) &&
+ signal.value === undefined &&
+ signal.workflowId === undefined &&
+ signal.to === undefined &&
+ signal.payload === undefined
+ );
}
if (signal.type === 'metadata_clear') {
- return true;
+ return (
+ signal.key === undefined &&
+ signal.value === undefined &&
+ signal.workflowId === undefined &&
+ signal.to === undefined &&
+ signal.payload === undefined
+ );
}Also applies to: 74-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts` around lines 55 -
60, The metadata_delete and metadata_clear branches currently accept extra
unrelated fields; update their validation to reject extra properties by ensuring
the signal object only contains the allowed keys: for metadata_delete require
signal.type === 'metadata_delete' AND isValidMetadataSignalKey(signal.key) AND
Object.keys(signal) is exactly ['type','key'], and for metadata_clear require
signal.type === 'metadata_clear' AND Object.keys(signal) is exactly ['type'] (do
the same corresponding stricter check in the other branch around lines 74-75).
Use the existing isValidMetadataSignalKey helper and the signal.type
discriminator when implementing the exact-key checks so the accepted shape
matches defaultMessage().
…fixes NV-7501
Complete the metadata CRUD API on agent context:
- get(key) reads from local state (reflects set/delete within the same handler)
- delete(key) removes a key and emits a delete signal
- clear() resets metadata to {} and emits a clear signal
- current provides a readonly snapshot of the metadata state
Refactor API layer to normalize metadata actions at the DTO boundary
(MetadataOp discriminated union) instead of raw signal arrays.
Co-authored-by: Cursor <cursoragent@cursor.com>
4d40288 to
9e6db08
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts (1)
95-102:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExtra-field rejection on
delete/clearactions still missing.When
action === 'delete'the validator only checkskey, and whenaction === 'clear'it returnstrueunconditionally — so payloads with strayvalue,workflowId,to, orpayloadfields still pass validation and get silently dropped downstream. The same concern was raised on a previous revision of this file (back when the discriminator was ontype); it carries over to the action-based shape and is worth tightening so the accepted contract matchesdefaultMessage().🛡️ Proposed fix
if (signal.type === 'metadata') { const action = signal.action ?? 'set'; - if (action === 'set') return isValidMetadataSignalKey(signal.key) && signal.value !== undefined; - if (action === 'delete') return isValidMetadataSignalKey(signal.key); - if (action === 'clear') return true; + const noTriggerFields = signal.workflowId === undefined && signal.to === undefined && signal.payload === undefined; + if (action === 'set') { + return isValidMetadataSignalKey(signal.key) && signal.value !== undefined && noTriggerFields; + } + if (action === 'delete') { + return isValidMetadataSignalKey(signal.key) && signal.value === undefined && noTriggerFields; + } + if (action === 'clear') { + return signal.key === undefined && signal.value === undefined && noTriggerFields; + } return false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts` around lines 95 - 102, The metadata-signal branch currently allows extra fields for action === 'delete' and always returns true for action === 'clear'; update the validator in the signal.type === 'metadata' branch to explicitly reject payloads that contain disallowed keys (e.g., value, workflowId, to, payload) for delete and clear actions. Keep using isValidMetadataSignalKey(signal.key) for key validation, but add a check (e.g., allowedKeys set or Object.keys(signal) subset test) so delete only permits the minimal keys (type, action, key) and clear permits only (type, action) (or the minimal contract implied by defaultMessage()), returning false if any extra fields are present.
🧹 Nitpick comments (2)
packages/framework/src/resources/agent/agent.context.ts (1)
295-317: 💤 Low valueLGTM — local snapshot + signal queue interleave correctly.
_metadataStateis seeded fromrequest.conversation.metadata, mutated in place byset/delete, and reassigned to{}byclear. Because each op also pushes its corresponding signal in order, server-side replay of[set a, clear, set b]yields{ b }, which matches whatcurrentreports locally.current's shallow copy keeps the readonly contract honest at runtime, not just in TS.Nit (optional):
ctx.conversation.metadatais a snapshot from request time and will not reflect post-mutation state — consider a one-line note in themetadataJSDoc steering users tometadata.current/metadata.get(key)for the live view.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/framework/src/resources/agent/agent.context.ts` around lines 295 - 317, Add a one-line JSDoc note to the metadata property explaining that ctx.conversation.metadata is a snapshot taken from the incoming request and will not reflect mutations, and direct users to use metadata.current or metadata.get(key) for the live view; update the JSDoc for the metadata object (referencing metadata, metadata.current, and metadata.get) with that concise guidance.apps/api/src/app/agents/services/agent-conversation.service.ts (1)
345-360: 💤 Low valueOptional: make the op switch exhaustive.
The switch covers the three current
MetadataOpactions but has nodefault— if a new action is later added toMetadataOp(e.g.merge), it will be silently skipped here while still consuming the description slot inconsistently. An exhaustiveness assertion catches this at compile time.♻️ Proposed defensive default
case 'clear': merged = {}; descriptions.push('(cleared)'); break; + default: { + const _exhaustive: never = op; + + throw new Error(`Unsupported metadata op: ${JSON.stringify(_exhaustive)}`); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/app/agents/services/agent-conversation.service.ts` around lines 345 - 360, The switch over params.ops (handling op.action in the loop that updates merged and descriptions) is not exhaustive for the MetadataOp union; add a defensive default branch to surface new actions at compile time/run time: implement an assertUnreachable(value: never): never helper (or throw a descriptive error) and call it in the switch default so any unknown op.action (e.g., a future "merge") triggers a compile-time type error or a clear runtime error instead of being silently ignored; update the switch in the agent conversation logic to include this default and reference op.action, merged, and descriptions when adding the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.ts`:
- Around line 358-378: The switch in handle-agent-reply.usecase.ts currently
uses "case 'set': default:" which treats any unknown action as a 'set'; change
the switch so 'set' is its own case and the default branch throws a
BadRequestException for unknown actions (include the invalid action value in the
message). Ensure you still validate signal.key with isValidMetadataSignalKey and
signal.value presence in the 'set' case, and leave the existing 'clear' and
'delete' handling (ops push and BadRequestException uses) intact so only truly
unknown actions are rejected.
In `@packages/framework/src/resources/agent/agent.test.ts`:
- Around line 232-233: Add four unit tests in agent.test.ts that exercise the
public API methods metadata.delete and metadata.clear as described in the PR:
(1) a test asserting metadata.delete emits a signal with { type: 'metadata',
action: 'delete', key, value: undefined } (or appropriate payload) and that
subsequent flush behavior remains correct; (2) a test asserting metadata.clear
emits a { type: 'metadata', action: 'clear' } signal and that flush-only clear
behaves as described; (3) a mixed-order test combining metadata.set,
metadata.delete, and metadata.clear and asserting signals appear in the expected
order in replyBody.signals; and (4) a test for a “clear-only flush” case that
invokes metadata.clear without intermediate flushes and verifies only the clear
signal is present. Reuse the existing test harness and the same
replyBody.signals assertions pattern (replace or extend the existing expect
lines around replyBody.signals) and name tests to mirror the PR description so
reviewers can find them quickly.
---
Duplicate comments:
In `@apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts`:
- Around line 95-102: The metadata-signal branch currently allows extra fields
for action === 'delete' and always returns true for action === 'clear'; update
the validator in the signal.type === 'metadata' branch to explicitly reject
payloads that contain disallowed keys (e.g., value, workflowId, to, payload) for
delete and clear actions. Keep using isValidMetadataSignalKey(signal.key) for
key validation, but add a check (e.g., allowedKeys set or Object.keys(signal)
subset test) so delete only permits the minimal keys (type, action, key) and
clear permits only (type, action) (or the minimal contract implied by
defaultMessage()), returning false if any extra fields are present.
---
Nitpick comments:
In `@apps/api/src/app/agents/services/agent-conversation.service.ts`:
- Around line 345-360: The switch over params.ops (handling op.action in the
loop that updates merged and descriptions) is not exhaustive for the MetadataOp
union; add a defensive default branch to surface new actions at compile time/run
time: implement an assertUnreachable(value: never): never helper (or throw a
descriptive error) and call it in the switch default so any unknown op.action
(e.g., a future "merge") triggers a compile-time type error or a clear runtime
error instead of being silently ignored; update the switch in the agent
conversation logic to include this default and reference op.action, merged, and
descriptions when adding the assertion.
In `@packages/framework/src/resources/agent/agent.context.ts`:
- Around line 295-317: Add a one-line JSDoc note to the metadata property
explaining that ctx.conversation.metadata is a snapshot taken from the incoming
request and will not reflect mutations, and direct users to use metadata.current
or metadata.get(key) for the live view; update the JSDoc for the metadata object
(referencing metadata, metadata.current, and metadata.get) with that concise
guidance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4c9aa480-def2-4a7c-8e96-4f5ae5f36d19
📒 Files selected for processing (7)
apps/api/src/app/agents/dtos/agent-reply-payload.dto.tsapps/api/src/app/agents/services/agent-conversation.service.tsapps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.tspackages/framework/src/resources/agent/agent.context.tspackages/framework/src/resources/agent/agent.test.tspackages/framework/src/resources/agent/agent.types.tspackages/novu/src/commands/init/templates/app-agent/ts/app/novu/agents/support-agent.tsx
… tests - Separate `case 'set'` from `default` in normalizeMetadataOps so unknown actions throw BadRequestException instead of silently becoming a set op - Add tests for ctx.metadata.delete(), clear(), get(), current, and mixed signal ordering Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.ts (1)
258-263: 💤 Low valueInline metadata signal type duplicated; consider reusing the framework type.
The anonymous shape
{ type: 'metadata'; action?: string; key?: string; value?: unknown }is declared here and again at line 351 innormalizeMetadataOps. Given the PR widensMetadataSignalinpackages/framework/src/resources/agent/agent.types.tsto carry the action discriminant, reusing/importing that type (or defining a single local alias) would keep the API and the usecase from drifting and remove theascast on line 258 entirely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.ts` around lines 258 - 263, The inline anonymous metadata shape is duplicated (used when creating rawMetadata and again in normalizeMetadataOps); replace both ad-hoc declarations by importing and using the framework's MetadataSignal type (which now includes the action discriminant) or create a single local type alias, update rawMetadata to use that concrete type (removing the "as" cast), and change normalizeMetadataOps to accept/handle MetadataSignal so both sites share the same type definition.packages/framework/src/resources/agent/agent.test.ts (1)
964-996: 💤 Low value
get/current/deletelocal-state test is correct, but asserts only the post-delete snapshot.The test verifies
get('score')returns the just-set value and thatcurrentis empty afterdelete. Consider also assertingcurrentreflects the set before the delete (e.g. capture a second snapshot betweensetanddelete) so a regression wherecurrentalways returns{}would still fail this single test. Also worth assertinggetResultafterdeleteisundefinedto lock in the read-after-delete contract.Note:
currentSnapshotis declared without an initializer and read with!on line 995 — this is intentional given the repo's Biome rule against= undefinedinitializers, so leave it as-is.Based on learnings from PR 11003 (Biome's
noUselessUndefinedInitializationrule strips= undefinedinitializers — prefer structural fixes / non-null assertions over= undefined).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/framework/src/resources/agent/agent.test.ts` around lines 964 - 996, The test currently only asserts the post-delete metadata snapshot; update the "should track local state across get, set, delete, and current" test to also capture and assert the metadata state immediately after setting and before deletion and to assert the value returned by get after delete is undefined: inside the agent('test-bot') onMessage handler (the function that calls ctx.metadata.set/get/delete/current) add a second snapshot variable (e.g. preDeleteSnapshot) right after ctx.metadata.set('score', 42) and before ctx.metadata.delete('score'), assert that preDeleteSnapshot equals { score: 42 }, then after ctx.metadata.delete('score' ) assert ctx.metadata.get('score') is undefined and keep the existing post-delete currentSnapshot assertion that it equals {} to lock the read-after-delete and current-before-delete behaviors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@apps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.ts`:
- Around line 258-263: The inline anonymous metadata shape is duplicated (used
when creating rawMetadata and again in normalizeMetadataOps); replace both
ad-hoc declarations by importing and using the framework's MetadataSignal type
(which now includes the action discriminant) or create a single local type
alias, update rawMetadata to use that concrete type (removing the "as" cast),
and change normalizeMetadataOps to accept/handle MetadataSignal so both sites
share the same type definition.
In `@packages/framework/src/resources/agent/agent.test.ts`:
- Around line 964-996: The test currently only asserts the post-delete metadata
snapshot; update the "should track local state across get, set, delete, and
current" test to also capture and assert the metadata state immediately after
setting and before deletion and to assert the value returned by get after delete
is undefined: inside the agent('test-bot') onMessage handler (the function that
calls ctx.metadata.set/get/delete/current) add a second snapshot variable (e.g.
preDeleteSnapshot) right after ctx.metadata.set('score', 42) and before
ctx.metadata.delete('score'), assert that preDeleteSnapshot equals { score: 42
}, then after ctx.metadata.delete('score' ) assert ctx.metadata.get('score') is
undefined and keep the existing post-delete currentSnapshot assertion that it
equals {} to lock the read-after-delete and current-before-delete behaviors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c56ea4fc-10ce-4b34-b1b8-e33afacb1eea
📒 Files selected for processing (2)
apps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.tspackages/framework/src/resources/agent/agent.test.ts
Summary
Complete the metadata CRUD API on the agent context:
ctx.metadata.get(key)— read a value from local state (reflectsset()/delete()within the same handler)ctx.metadata.delete(key)— remove a key and emit a delete signalctx.metadata.clear()— reset metadata to{}and emit a clear signalctx.metadata.current— readonly snapshot of the current metadata statectx.metadata.set(key, value)— existing, now emits an explicitaction: 'set'in the signalAPI-side refactoring
actionfield (set/delete/clear) on metadata signals at the wire boundaryMetadataOpdiscriminated union replaces raw signal arrays,updateMetadatahandles all 3 actions via switchnormalizeMetadataOpsreplacesvalidateMetadataSignalKeys, converting optional wireactionto requiredMetadataOpTemplate
support-agent.tsxusesctx.metadata.get('topic')instead ofctx.conversation.metadata?.topicTest plan
get()reflectsset()/delete()within the same handler invocation'set')What changed
Agent metadata handling was completed with full CRUD on the runtime context: ctx.metadata.get(key), delete(key), clear(), set(key, value) (now emits explicit 'set' action), and a readonly ctx.metadata.current snapshot. These operations enqueue metadata signals with explicit actions (set/delete/clear) so they are batched and persisted via the existing reply/flush pipeline; validation and server-side handling were updated to apply ops in order and keep backward compatibility for clients that omit action.
Affected areas
shared / framework: AgentContext.metadata now exposes get, set, delete, clear, and readonly current; internal state is updated locally and emits metadata signals containing an action to be flushed with replies; tests updated to assert new behaviors and signal ordering.
api: Signal DTOs and validation were extended to accept an action (set|delete|clear); normalizeMetadataOps converts optional wire actions into required MetadataOp entries and updateMetadata applies set/delete/clear ops in order, including clear resetting metadata to {}.
templates (novu CLI): Example support agent reads topic from ctx.metadata instead of ctx.conversation.metadata.
Key technical decisions
Testing
Added and updated unit tests in the framework to cover ctx.metadata.get(), delete(), clear(), current, mixed signal ordering, and flush behavior; API validation updated accordingly. Framework tests reported passing in the branch (313 tests, 0 type errors).